refactor(scatter-plot): decompose the component & WebGL renderer into focused modules#291
Merged
Conversation
…24,25,26,27) Lock load-bearing WebGLRenderer + component cache/memo/race behavior before any structural refactor touches the two god-files. Test-only; zero production diff. - New mock-WebGL2 harness (test-support/mock-webgl2.ts) with context-unavailable, program-link-fail, missing-float-extensions, framebuffer-incomplete toggles. - WebGLRenderer locks: sampled-slot signature cache (F-02), init-failure no-op/no-throw graceful degradation (F-03), context loss/restore + gamma fallback (F-09). - Component locks: _scales same-length swap invalidation (F-22), numeric-recompute stale-job guard (F-23), duplicate-stack chunked compute job/cache guards (F-24), async tooltip-height measure race (F-25), _styleGettersCache lifecycle (F-26), _getVisibilityModel 8-field memo key (F-27). Each lock passes on the unmodified tree and was proven non-vacuous (flip-one-expectation => FAIL => revert => PASS). F-09 missing-extensions case locks the true warn-count (0, not the plan sketch's 1: ensureGL clears gammaPipelineAvailable before handleGammaFallback, whose guard then bypasses console.warn). 31 new cases; core vitest 60->69 files, 1064->1095. Gates green: type-check, core+utils vitest, build, lint (0 errors), knip.
…-43,F-39,F-01,F-10) - F-43: destroy() now calls dispose() — single GPU-teardown owner, freeing buffers/VAO/textures/programs/framebuffer that the listener-only teardown leaked. - F-39: delete the unreachable internal handleContextRestored handler + its webglcontextrestored listener (dead under the rebuild-on-loss strategy); drop the now-unused EMPTY_PLOT_DATA import. - F-01 (SANCTIONED VISIBLE CHANGE): route programmatic context loss to recovery by moving onContextLost?.() into markContextLost() (inside its idempotency guard) and dropping the direct call from the DOM handler. Every loss path (DOM + programmatic ensureGL/isContextLost) now fires recovery exactly once instead of latching blank. - F-10: guard the recovery microtask in _handleWebglContextLost with a generation token + isConnected check so a detached element (route change) or a superseded loss no longer reconstructs a renderer on a detached node. Tests: new webgl-renderer.lifecycle.test.ts (F-43/F-39/F-01); F-10 case in scatter-plot.test.ts (connect-then-disconnect variant — the verbatim plan sketch hung on updateComplete for a never-appended Lit element); two B7 context-loss cases that characterized the deleted restore handler reconciled to the new no-internal-restore reality. Gates: type-check, core vitest (1100), utils vitest, build, lint (0 err), knip; e2e dataset-recovery 5/5; visual diff 6/6 (0 drift); F-01 principled oracle PASS (forced WEBGL_lose_context -> rebuild returns pixel-identical, non-blank canvas).
…-38,20,41,42,37,14,49,58,08,15) Collapse the duplicated live-vs-offscreen WebGL pipeline in webgl-renderer.ts into shared webgl/renderer substrate helpers, then rewire the offscreen export to consume them (webgl-renderer.ts: -217 net lines). New pure helpers (each + unit test, node-env mock GL): - point-locations.ts resolvePointLocations (F-38) — collapses 4 hand-spelled location records - point-attributes.ts POINT_ATTRIBUTE_LAYOUT + setupAttributes (F-20) - framebuffer.ts createLinearFramebuffer + destroyFramebuffer(gl,fb) (F-41) - render-target.ts bindAndClearTarget + setPointBlendState (F-42) + drawPoints (F-15) - gamma-quad.ts QUAD_VERTICES + drawGammaQuad (F-37) - stage-point.ts stagePoint(10-arg, pre-scaled) + StagePointArrays/StagePointStyle (F-14) - data-extent.ts computeExtent/computePaddedExtent + DATA_EXTENT_PADDING (F-49) - viewport-defaults.ts DEFAULT_VIEWPORT_WIDTH/HEIGHT (F-58) + PointAttribLocations/PointUniformLocations named types in webgl/types.ts. F-08: omnibus rewire of initializeOffscreenContext/prepareOffscreenBufferData to the helpers; deletes createOffscreenLinearFramebuffer + renderOffscreenGammaCorrection. F-14 color-only live staging loop kept inline (documented exemption). F-15 (SANCTIONED VISIBLE CHANGE): offscreen export gains the live two-pass selection blend (records selectedStartIndex; both live renderPoints and renderOffscreenPoints now route through the shared drawPoints). Live rendering is a pure extraction (byte-identical). Gates: type-check, core vitest (1121), utils vitest, build, lint (0 err), knip (dead code removed); e2e figure-editor 4/4; visual diff 7/7 (0 live drift); F-15 export==live oracle PASS (two-pass branch exercised; luma conserved to 0.1%; residual is a pre-existing margin framing offset, not a blend delta).
…0,F-61,F-29) Shrink the 2105-line WebGLRenderer to a 1147-line facade (-958) by extracting its natural seams; strictly behavior-preserving (no sanctioned change). - F-50: route the remaining framebuffer-teardown triples through B3's existing destroyFramebuffer(gl,fb) (no new module). - F-61: GLResources holder owns the GPU inventory (programs, 6 vertex buffers + quad, VAO, label texture, linearFramebuffer) with createAll/validate/deleteAll/reset; dirty-flag cache signatures stay on the renderer. validate() is a byte-faithful mirror of the original isRendererStateValid (deliberately does NOT check quadBuffer or linearFramebuffer — adding those would change when ensureGL resets). - F-29: split into ContextLossController (single webglcontextlost listener per the post-B1 model; markLost idempotent -> onContextLost) and ExportRenderer (offscreen subsystem + inset math, consuming the B3 substrate, preserving the F-15 two-pass); shared point/gamma shader sources factored into export-shaders.ts. The facade preserves every public renderer method signature (the scatter-plot<->renderer contract). Gates: type-check, core vitest (1144), utils vitest, build, lint (0 err), knip; e2e dataset-recovery 5/5 + figure-editor 4/4; visual 6/6 (0 drift); F-01 + F-15 oracles both still PASS (split perturbs neither sanctioned behavior).
…59,54,13,40,17,04,18) De-duplicate and harden the scatter-plot.ts data-derivation/caching surface; behavior-preserving except two sanctioned changes. - F-60/F-59/F-54 (trivial trio): single numeric-column read; DEFAULT_NUMERIC_BIN_COUNT const; createScales gets an explicit ScalePair|null return (ScalePair exported from @protspace/utils — utils can't import core webgl/types; the webgl/types alias stays a transient duplicate B12 collapses) and scatter-plot drops the redundant cast. - F-13: sliceVisualizationDataByIndices(data, keptIndices) shared slicer in @protspace/utils (B10 reuses it); _getCurrentDisplayData + getCurrentData isolation branch delegate to it, which also realigns annotation_scores/annotation_evidence to keptIndices (latent fix, invisible to every current consumer; INV-04 preserved). - F-40: memoize the filtered display-data rebuild (keyed by ref on materialized + filteredProteinIds + filtersActive + selectedProjectionIndex + projectionPlane); value-identical (same object ref on hit, full slice retained). - F-17 (SANCTIONED bug fix, >=1M only): _buildQuadtree bumps a _quadtreeGeneration folded into the virtualization cacheKey + invalidates + renders, so un-hidden points reappear without a pan/zoom. <1M rendered pixels unchanged. - F-04: extract NumericRecomputeRunner (job-id race per B7/F-23 + start/end events + running mirror + cancel-on-disconnect), all preserved exactly. - F-18: split updated() into ordered named effects sharing one INV-11 geometry predicate; call order + every guard preserved verbatim. Gates: type-check, core vitest (1164), utils vitest (301), build, lint (0 err), knip; e2e numeric-binning 41/41 + isolation-dataset-swap 2/2; visual 6/6 (0 drift); F-01 + F-15 oracles still PASS. F-17 >=1M path validated by its committed unit-mechanism oracle (scatter-plot.b6.test.ts); large-bundle fixture is local-only/absent here.
…xtraction (F-53,51,52,30,32,36,06) Lift the ~450-line inline duplicate-stack overlay feature out of scatter-plot.ts into a cohesive DuplicateStackOverlayController; strictly behavior-preserving (no carve-out). scatter-plot.ts ~3017 -> 2486 lines. - F-53: ViewportDuplicateStack named type replaces 8 inline record literals. - F-51: computeViewportWindow + buildViewKey pure helpers (3 duplicated viewport blocks + 2 viewKey templates collapsed). - F-52: cullAndCapStacks viewport-cull + top-N cap helper (replaces the inline filter + _capDuplicateStacksForRendering; DUPLICATE_BADGES_MAX_VISIBLE single source). - F-30: DuplicateBadgesCanvasRenderer — the Canvas2D badge engine out of the component (byte-identical geometry/style constants). - F-32: SpiderfyLayer — SVG ring build + pointer-capture click-synthesis (ring radius/ angles + 16px/700ms click thresholds verbatim); event dispatch STAYS on the host via onActivate/onHover/onHoverEnd callbacks (INV-05/INV-03). - F-36: production grouping reuses the shared buildDuplicateStacks helper (one impl). - F-06: compose DuplicateStackOverlayController owning all state/schedulers/compute; component holds one _dupOverlay field and forwards; the B7/F-24 + duplicate-overlay Lock probes re-pointed to controller accessors with the same asserted contracts. Gates: type-check, core vitest (1187), utils vitest (301), build, lint (0 err), knip; visual 6/6 (0 drift, feature off by default) + F-01 + F-15 oracles still PASS. Duplicate- stack UI is opt-in with no e2e project; unit + component-characterization coverage is the behavioral guard. (Minor: duplicate-badges-canvas-renderer keeps a local render-subset type; collapse onto duplicate-stack-types deferred to B12's sweep.)
Lift the fused d3 zoom/brush/lasso + RAF interaction layer out of scatter-plot.ts into a cohesive PlotInteractionController; strictly behavior-preserving (event dispatch stays on the host per INV-03/INV-05). scatter-plot.ts 2486 -> 2293 lines. - F-48: demote _transform from @State to a plain field (render() never reads it) — removes the redundant per-zoom-frame updated()->_renderPlot() double-render; the zoom RAF + d3 attr() transform keep the visual byte-identical. - F-28: unify the duplicated hover/click hit-test into one pickInteractivePointAt (15px search radius, /3 point radius); both hover and click resolve the identical point. - F-07: PlotInteractionController owns the d3 zoom/brush/lasso lifecycle, the SVG groups, and the zoom/lasso RAF handles, driven by a narrow PlotInteractionHost bridge; the host keeps _slotsToInteractiveIds + _commitSelection + all event dispatch (INV-03/05), plus _hoverRaf/_quadtreeRebuildRafId. teardown() interrupts the reset transition (incidental to F-12, which B2 owns — not claimed here). Thin _handleLassoEnd/_handleBrushEnd host shims keep scatter-plot.test.ts byte-identical. Tests: new plot-interaction-controller.test.ts, scatter-plot.pick.test.ts, scatter-plot.transform-reactivity.test.ts. The committed brush-selection.spec.ts (which drives the brush/zoom programmatically through the now-relocated _brush/_brushGroup/ _svgSelection/_zoom) is re-pointed to plot._interaction.* — same asserted contracts. Gates: type-check, core vitest (1196), utils vitest (301), build, lint (0 err), knip; e2e brush-selection 8/8 + multi-annotation-tooltip 3/3; visual 6/6 (0 drift, incl. the re-driven zoomed + selection views matching baseline) + F-01 + F-15 oracles still PASS.
Lift the tooltip-positioning math out of scatter-plot._getTooltipStyle into a pure, unit-testable tooltip-position.ts (computeTooltipStyle + named constants TOOLTIP_EDGE_PADDING=15, TOOLTIP_MAX_WIDTH=350, TOOLTIP_ANCHOR_OFFSET_X=15, TOOLTIP_ANCHOR_OFFSET_Y=60, TOOLTIP_FALLBACK_HEIGHT=160), mirroring the tooltip-height-estimate precedent. The component delegates only the pure math; the no-tooltipData early-return + height-resolution (_tooltipHeight ?? _estimateTooltipHeight) are preserved. Byte-identical CSS output across all branches (interior, right-edge flip, left/top clamps), proven by the parameterized computeTooltipStyle unit suite. Strictly behavior-preserving: protein-hover dispatch (INV-03 bubbles, not composed), INV-05 detail shape, the async measure-token guard, and the four _tooltip* @State fields are untouched; the finding's optional state-object clause is deliberately not exercised (would change Lit reactivity keys — no carve-out). Gates: type-check, core vitest (1203), utils vitest (301), build, lint (0 err), knip; e2e multi-annotation-tooltip 3/3; visual 6/6 (0 drift) + F-01 + F-15 oracles still PASS.
Collapse the byte-identical reprocess+rebuild-quadtree+invalidate-renderer-caches+refresh- signature+requestUpdate+deferred-renderPlot sequence duplicated in isolateSelection() and resetIsolation() into one private _reprocessAndRefresh(); both callers route through it. resetIsolation keeps _lastDataRef = null BEFORE the call (the only divergence, forcing the full-rebuild path). Strictly behavior-preserving: all event dispatches (data-isolation, data-isolation-reset, data-change, auto-disable-selection) and their detail shapes are untouched (INV-05), and getCurrentData stays the constrained isolated view (INV-04). The plan's optional Step 8 (getCurrentData reslice via sliceVisualizationDataByIndices) was already landed by B6/F-13 — verified present and skipped here (getCurrentData untouched). Tests: render-refresh sequence + single-shared-impl characterization in scatter-plot.isolation.test.ts (the _lastDataRef-null-before-reprocess divergence pinned). Gates: type-check, core vitest (1206), utils vitest (301), build, lint (0 err), knip; e2e isolation-dataset-swap 2/2; visual 6/6 (0 drift) + F-01 + F-15 oracles still PASS.
…F-12,F-16,F-21) Construct the WebGLRenderer in exactly one place and make every teardown path safe; strictly behavior-preserving on the connected happy path. - F-35+F-11: extract _createWebglRenderer() (canvas + scale/transform/config closures + the 7-getter style bundle + _handleWebglContextLost) and route BOTH firstUpdated and _updateSizeAndRender through it. firstUpdated no longer unconditionally re-constructs a second renderer (the previous double-construction orphaned renderer #1). - F-16: track _commitSelectionRafId and cancel it in disconnectedCallback, so a deferred selection commit whose RAF resolves after detach never dispatches — implemented by cancellation (matching F-05/F-10), NOT an isConnected body-guard, so the connected commit flow stays byte-identical. - F-21: _renderWebGL early-returns when _webglRenderer is null (drops the two ! asserts), converting a post-context-loss TypeError into a no-op. - F-05: already satisfied by B6/F-04 (NumericRecomputeRunner.cancel() bumps the job id + cancels the RAF; disconnectedCallback calls it) — added a characterization lock, no production change; did not re-introduce a host job-id field. - F-12: already satisfied by B8 (PlotInteractionController.teardown() interrupts the 750ms resetZoom transition; disconnectedCallback calls teardown) — added a controller-level interrupt characterization, no production change; did not re-introduce a host svg field. Tests: new scatter-plot.lifecycle.test.ts (5 findings) + an F-12 teardown-interrupt test on PlotInteractionController. scatter-plot.test.ts unchanged. Gates: type-check, core vitest (1212), utils vitest (301), build, lint (0 err), knip; e2e dataset-recovery + brush-selection + numeric-binning 54/54; visual 6/6 (0 drift) + F-01 + F-15 oracles still PASS.
…F-19,F-31,F-57,F-47,F-46) Tighten the Lit reactivity and event/type contracts; strictly behavior-preserving. - F-19+F-31: new legend/legend-mapping-events.ts (typed LegendColorMappingDetail/ LegendZOrderDetail per INV-07 + isLegend* runtime key-validation guards). The two legend handlers consume the typed details and early-return on malformed input; _zOrderMapping/_colorMapping/_shapeMapping are demoted from @State to plain fields (read pull-based in the style-getter closures, not in render()), so a legend mapping change renders ONCE via the imperative path (the INV-08 colorOnly branch intact) instead of twice — visually identical. - F-57: drop the redundant explicit requestUpdate() in NumericRecomputeRunner (start + end); the setRunning() @State mirror (_numericRecomputeRunning) already schedules the Lit update. (The plan's L846/L904 were stale — the calls moved into the runner in B6.) - F-47: type IScatterplotElement.config + scatterplot-sync-controller.updateConfig as Partial<ScatterplotConfig> (compile-time tightening; @ts-expect-error guard added). - F-46: remove the unconsumed public numeric-recompute-start/-end CustomEvents (re-verified zero listeners; absent from INV-05). The runner's job-id stale-job guard + busy state are kept; the three committed tests that asserted the events (runner unit, B7/F-23, B2/F-05) are re-characterized to observe last-write-wins via the _numericRecomputeRunning mirror. Gates: type-check, core vitest (1227), utils vitest (301), build, lint (0 err), knip; e2e numeric-binning 41/41; visual 6/6 (0 drift) + F-01 + F-15 oracles still PASS.
…-56,F-62) Strict no-op: delete confirmed-dead, unconsumed symbols (verified via knip + repo-wide grep) and collapse two transient duplicate types left by earlier batches. Every live render path stays byte-identical. - F-44: remove the dead per-point stroke plumbing end-to-end (getStrokeColor/getStrokeWidth getters + WebGLStyleGetters members + the wiring keys + the _getStrokeColor/_getStrokeWidth wrappers). The fragment shader's hardcoded strokeWidth=0.15 (the only stroke value the GPU uses) is untouched — that is what makes this a no-op. - F-45: remove the unused VisibilityModel.tierOf + exported DisplayTier (the opacity/depth carriers getOpacity/getDepth/opacityOf/baseOpacityOf/isInteractive are untouched). - F-55: remove the unused public getGamma/setGamma accessors (the gamma field, all its read sites, and getEffectiveGamma are kept). - F-56: remove the deprecated no-op setSelectedAnnotation. - F-62: rewrite the stale 'BIT-FOR-BIT replica' visibility-model comment to single-authority phrasing. - Collapse (recon #7): webgl/types.ts ScalePair now re-exports the @protspace/utils ScalePair (single canonical definition; B6/F-54's transient duplicate removed). - Collapse (from B5): duplicate-badges-canvas-renderer's local render-subset type folded onto RenderDuplicateStack = Pick<ViewportDuplicateStack,...> in duplicate-stack-types (single ViewportDuplicateStack definition). Gates: type-check, core vitest (1231), utils vitest (301), build, lint (0 err), knip (reduced unused surface), dead-ref grep clean; e2e figure-editor + brush-selection 12/12; visual 6/6 (0 drift) + F-01 + F-15 oracles still PASS.
Post-merge review fixes for the interaction layer: - Restore the pre-data guard dropped in the controller extraction: add PlotInteractionHost.hasScales() and re-gate updateSelectionMode() so enabling selection on an empty/loading plot is a no-op again (matches main). - Make the host the single owner of the d3 transform: drop the controller's parallel _transform, add host.getTransform(); applyZoom writes via onTransform() before any in-handler read. - Remove dead host-side lasso state (_isLassoing/_lassoVertices/_lassoPath/_handleLassoEnd/_clearLassoVisual); migrate the two unit tests to drive the live controller path.
Deduplicate WebGL code shared by the live and offscreen-export paths: - Extract bindPointDrawState() (render-target.ts) for the shared point uniform/texture/VAO setup; used by renderPoints and renderOffscreenPoints. - Extract buildPaintOrder() (point-staging.ts) as the canonical painter-order + selectedStartIndex staging; converge the export path onto it so export==live is structural (F-15 oracle stays green). Flatten the selectedStartIndex ternary. - Make the blend/depth precondition local to the point draw (setPointBlendState inside bindPointDrawState). - Add point-staging unit tests.
- Extract pointInWindow() (duplicate-stack-viewport.ts); replace the inclusive-bounds predicate copy-pasted at 3 sites. - Clear expandedAnchor on every collapse path (symmetric with closeExpanded). - Merge the two byte-identical updateOverlays guard branches. - Extract renderBadgesForViewport() shared by updateOverlays and redrawBadgesOnly.
Remove the _running field and isRunning() method (no production caller — the host tracks busy state via runningAnnotation()); update tests to assert running state via runningAnnotation().
Organize the flat scatter-plot/ helper files into feature folders so the directory is easier to navigate. No behavior change — only file moves (via git mv, history preserved) plus relative-import path updates.
New layout (entry, config, styles, perf, integration tests, and the webgl/ subtree stay at the root):
- tooltips/ protein-tooltip(+styles,+helpers), protspace-tips(+styles), tooltip-position, tooltip-height-estimate
- duplicate-stacks/ duplicate-badges-canvas-renderer, duplicate-stack-{helpers,overlay-controller,types,viewport}, spiderfy-layer
- interaction/ plot-interaction-controller, quadtree-index
- styling/ visibility-model, style-getters, numeric-recompute-runner
- projection-metadata/ projection-metadata(+styles)
Follow-up fixes from a deep code review of the scatter-plot refactor: - Cache gamma-correction uniforms instead of re-querying them per frame - Guard the duplicate-stack viewport cache key against a k=0 divergence - Use a spread-free reduce for legend zMax (avoids Math.max(...) overflow on very large legends) - Share point-style staging between the full rebuild and the color-only recolor path via a new stagePointStyle helper - Drop a dead picking host-interface member and the unused static export data-extent seam; dedupe the duplicate-stack collapse helpers - Consolidate the 0.05 plot-padding constant into @protspace/utils - Restore stubbed globals after the numeric-recompute runner test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
scatter-plot.tsand itswebgl-renderer.tshad grown into two files that owned almost everything: d3 zoom/brush, tooltips, the duplicate-stack overlay, styling, plus the full WebGL pipeline and a parallel offscreen-export path. They were hard to test in isolation, and the live and export render paths each carried their own copy of the painter-order and draw-state logic — close enough to look identical, loose enough to drift.This PR breaks both files into small, focused modules without changing what the component does.
How it stays safe
The refactor is behavior-preserving by construction:
What changed
tooltips/,duplicate-stacks/,interaction/,styling/).Compatibility
The public API is unchanged — the package exports the same custom element and type; only internals moved (file moves use
git mv, so history is preserved). The few intentional behavior changes are small, marked, and each backed by a dedicated test.Post-review hardening
A final code-review pass added a few more behavior-preserving guards and dedups, each covered by the unit + e2e suites: cached gamma-correction uniforms (was re-querying per frame), a viewport-key guard against a
k=0divergence, a spread-free legendzMax(avoidsMath.max(...)overflow on very large legends), shared point-style staging between the full rebuild and the color-only recolor, a consolidated plot-padding constant, and removal of a dead picking seam and the unused static export-extent method.